Conversation
….1.1, and TypeScript 5.9.2. Upgrade various packages including @c15t/translations, @playwright/test, and @types for improved compatibility and performance.
…ng linter rules and adding JSON formatting options. Remove deprecated performance metrics and collectors to streamline the codebase.
…tions, and cookie consent integration. Includes TypeScript support, necessary dependencies, and initial layout and page components.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis PR adds a new Ethyca benchmark app and configuration, updates many benchmark dependencies, revises Biome and root dev tools, and significantly removes the CLI’s benchmark/performance collection pipeline while performing minor CLI refactors and API surface reductions. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant N as Next.js App
participant RL as RootLayout
participant S as <Script beforeInteractive>
participant C as Fides CDN
participant W as window.Fides
U->>N: Request /
N->>RL: Render RootLayout
RL->>S: Inject inline loader
S->>C: Load fides.js (with query params)
C-->>S: fides.js
S->>W: Setup overrides & event listeners
W-->>S: FidesInitializing / FidesInitialized
S->>N: Continue render with children
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (3)
packages/cli/src/commands/db.ts (2)
26-29: Usecwdinstead ofcd &&and drop ineffectiveencodingto improve robustnessEmbedding paths in shell strings is brittle (spaces/special chars, cross‑platform quirks).
execSyncsupportscwd, which solves this cleanly. Also,encodingis ignored whenstdioisinherit.Apply this diff:
- execSync(`cd ${DB_PACKAGE_PATH} && pnpm drizzle-kit ${command}`, { - stdio: 'inherit', - encoding: 'utf-8' - }); + execSync(`pnpm drizzle-kit ${command}`, { + stdio: 'inherit', + cwd: DB_PACKAGE_PATH, + });
156-167: The try/catch here is also unreachable; either remove it or let the helper throwSame reasoning as in studio:
runDrizzleCommandexits on error, so this catch won’t run. Either:
- Remove the try/catch (lean on the helper to exit), or
- Change
runDrizzleCommandto throw and handle errors here.Minimal cleanup:
- try { - runDrizzleCommand('migrate'); - p.log.success('Migrations completed successfully!'); - p.outro('Database is now up to date.'); - } catch (error) { - p.log.error('Migration failed!'); - if (error instanceof Error) { - p.log.error(error.message); - } - process.exit(1); - } + runDrizzleCommand('migrate'); + p.log.success('Migrations completed successfully!'); + p.outro('Database is now up to date.');benchmarks/with-didomi/package.json (1)
2-2: Package name typo: “with-dodomi” should be “with-didomi”The folder is “with-didomi” but the package name is “with-dodomi”. This will cause confusion and possibly workspace linking issues.
Apply this fix:
- "name": "with-dodomi", + "name": "with-didomi",
🧹 Nitpick comments (14)
packages/cli/src/commands/db.ts (5)
39-39: Constrain thesubcommandtype to the known setThis prevents accidental invalid calls and improves DX.
-export async function dbCommand(subcommand?: string) { +export async function dbCommand( + subcommand?: 'push' | 'generate' | 'migrate' | 'studio' | 'status' +) {
41-41: Remove the artificial 1s delay for a snappier UXUnless there’s a specific need, the wait harms responsiveness.
- await setTimeout(1000); + // Removed artificial delay to keep the command responsive
43-43: Avoid hard-coding the version in the introThis will drift. Read it from package.json instead.
Example (outside this hunk):
// at top-level import { readFile } from 'node:fs/promises'; const pkg = JSON.parse(await readFile(join(findProjectRoot(), 'package.json'), 'utf-8')); const version = pkg.version ?? 'unknown'; // then p.intro(`${color.bgBlue(color.white(' database '))} ${color.dim(`v${version}`)}`);
8-10: DeriveDB_PACKAGE_PATHfrom project root for resilienceYou already have
findProjectRoot(). Using it avoids failures when CLI is run from a subdirectory.-const DB_PACKAGE_PATH = join(process.cwd(), 'packages', 'db'); +const DB_PACKAGE_PATH = join(findProjectRoot(), 'packages', 'db');
174-179: Change acknowledgement: bare catch matches the current usageGiven the catch body doesn’t use the error, switching to a bare
catchis fine and silences “unused variable” lint warnings. The larger concern is the unreachable catch explained above.packages/cli/src/utils/scoring.ts (1)
89-95: Lock down DEFAULT_SCORE_WEIGHTS to prevent accidental mutation.Good call making the weights internal. To avoid accidental runtime mutation, freeze the object and mark it readonly.
Apply:
-const DEFAULT_SCORE_WEIGHTS: ScoreWeights = { +const DEFAULT_SCORE_WEIGHTS: Readonly<ScoreWeights> = Object.freeze({ performance: 0.4, // 40% - Core Web Vitals and performance metrics bundleStrategy: 0.25, // 25% - First-party vs third-party, bundling approach networkImpact: 0.2, // 20% - Network requests, bundle size, third-party impact transparency: 0.1, // 10% - Open source, company info, tech stack disclosure userExperience: 0.05, // 5% - Layout stability, interaction responsiveness -}; +});benchmarks/with-didomi/package.json (1)
5-5: Consider specifying a uniquedevport to avoid collisionsMany benchmarks declare explicit, distinct ports. This package currently omits a port and may default to 3000, potentially colliding with others when running multiple apps. Consider setting a unique port.
Example change:
- "dev": "next dev --turbopack", + "dev": "next dev --port 3007 --turbopack",benchmarks/with-cookie-control/package.json (1)
13-15: Dependency bumps OK; watch for port collision with with-cookie-yesThis package and with-cookie-yes both use
--port 3001. If you ever run them concurrently for local testing, they’ll collide. If that’s not intended, ignore. Otherwise, consider switching one to a different port.Proposed change here to avoid conflict:
- "dev": "next dev --port 3001 --turbopack", + "dev": "next dev --port 3002 --turbopack",benchmarks/with-ethyca/next.config.ts (1)
3-5: Prefer “satisfies NextConfig” and enable strict mode (optional)Using “satisfies” preserves exact inference while type-checking keys. Adding reactStrictMode helps catch regressions early (harmless for benchmarks).
Apply:
-const nextConfig: NextConfig = { - /* config options here */ -}; +const nextConfig = { + reactStrictMode: true, + /* config options here */ +} satisfies NextConfig;benchmarks/with-ethyca/config.json (1)
31-31: Normalize license string to SPDX identifierThe license field should be an SPDX-compliant identifier.
- "license": "Apache-2.0 license", + "license": "Apache-2.0",benchmarks/with-ethyca/app/layout.tsx (2)
22-25: Guard for body availability and avoid potential early-timing issuesWhen running beforeInteractive, body should exist, but a small guard avoids edge cases.
- addEventListener("FidesInitialized", function () { + addEventListener("FidesInitialized", function () { var id = window.Fides?.experience?.experience_config?.id; - if (id) document.body.classList.add(id); + if (id) { + var bodyEl = document.body || document.documentElement; + bodyEl && bodyEl.classList && bodyEl.classList.add(id); + } });
44-49: Consider using defer to reduce main-thread blockingSetting async=false and defer=false forces immediate execution on append, which can block. If Fides doesn’t strictly require immediate execution at this exact phase, prefer defer for better page performance.
- s.async = false; - s.defer = false; + s.defer = true;If strict earliest execution is required for measurement fidelity, keep as-is.
biome.jsonc (2)
27-38: Secret scanning disabled; consider setting to warn instead of offTurning nursery.noSecrets off entirely can mask accidental credential commits.
- "noSecrets": "off", + "noSecrets": "warn",
57-61: noConsole globally off: consider scoping to CLI packages onlyIt’s reasonable for the CLI to use console, but benchmarks and libraries might still benefit from flagging stray console usage. Consider overrides to scope this rule.
Example override (apply outside this range, shown for clarity):
{ "overrides": [ { "include": ["packages/cli/**"], "linter": { "rules": { "suspicious": { "noConsole": "off" } } } } ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
benchmarks/with-ethyca/app/favicon.icois excluded by!**/*.icopnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
benchmarks/baseline/package.json(1 hunks)benchmarks/with-c15t-nextjs/package.json(1 hunks)benchmarks/with-c15t-react/package.json(1 hunks)benchmarks/with-cookie-control/package.json(1 hunks)benchmarks/with-cookie-yes/package.json(1 hunks)benchmarks/with-didomi/package.json(1 hunks)benchmarks/with-enzuzo/package.json(1 hunks)benchmarks/with-ethyca/app/layout.tsx(1 hunks)benchmarks/with-ethyca/app/page.tsx(1 hunks)benchmarks/with-ethyca/config.json(1 hunks)benchmarks/with-ethyca/next-env.d.ts(1 hunks)benchmarks/with-ethyca/next.config.ts(1 hunks)benchmarks/with-ethyca/package.json(1 hunks)benchmarks/with-ethyca/tsconfig.json(1 hunks)benchmarks/with-iubenda/package.json(1 hunks)benchmarks/with-ketch/package.json(1 hunks)benchmarks/with-onetrust/package.json(1 hunks)benchmarks/with-osano/package.json(1 hunks)benchmarks/with-usercentrics/package.json(1 hunks)biome.jsonc(1 hunks)package.json(1 hunks)packages/cli/package.json(1 hunks)packages/cli/src/commands/benchmark/benchmark-runner.ts(0 hunks)packages/cli/src/commands/benchmark/cookie-banner-detector.ts(1 hunks)packages/cli/src/commands/db.ts(1 hunks)packages/cli/src/commands/results.ts(1 hunks)packages/cli/src/index.ts(0 hunks)packages/cli/src/lib/benchmark-runner.ts(0 hunks)packages/cli/src/lib/collectors/cookie-banner-collector.ts(0 hunks)packages/cli/src/lib/collectors/index.ts(0 hunks)packages/cli/src/lib/collectors/network-monitor.ts(0 hunks)packages/cli/src/lib/collectors/resource-timing-collector.ts(0 hunks)packages/cli/src/lib/metrics/index.ts(0 hunks)packages/cli/src/lib/metrics/performance-aggregator.ts(0 hunks)packages/cli/src/lib/performance-enhanced.ts(0 hunks)packages/cli/src/lib/performance.ts(0 hunks)packages/cli/src/types/index.ts(0 hunks)packages/cli/src/utils/index.ts(0 hunks)packages/cli/src/utils/scoring.ts(2 hunks)packages/cli/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (13)
- packages/cli/src/utils/index.ts
- packages/cli/src/lib/collectors/index.ts
- packages/cli/src/commands/benchmark/benchmark-runner.ts
- packages/cli/src/lib/metrics/index.ts
- packages/cli/src/index.ts
- packages/cli/src/lib/benchmark-runner.ts
- packages/cli/src/types/index.ts
- packages/cli/src/lib/performance-enhanced.ts
- packages/cli/src/lib/collectors/network-monitor.ts
- packages/cli/src/lib/metrics/performance-aggregator.ts
- packages/cli/src/lib/performance.ts
- packages/cli/src/lib/collectors/cookie-banner-collector.ts
- packages/cli/src/lib/collectors/resource-timing-collector.ts
🔇 Additional comments (26)
packages/cli/src/commands/db.ts (1)
203-210: Verify the hard-coded database path assumptionThe status check assumes a file named
benchmarks.dbat the project root. If DB location or name changed with the package updates or differs per env, this may always warn. Consider reading from Drizzle config or ENV.Would you like a follow-up patch to resolve the path from
drizzle.config.tsor an env var and fall back tobenchmarks.db?packages/cli/tsconfig.json (1)
6-6: LGTM! DOM lib addition aligns with cookie-banner detection code.The addition of "DOM" to the lib array provides proper TypeScript typings for DOM APIs used in the cookie-banner detector and related DOM-dependent code in the CLI.
packages/cli/src/commands/benchmark/cookie-banner-detector.ts (1)
204-204: Good fix for NodeList iteration compatibility.Converting
mutation.addedNodesto an array usingArray.from()ensures cross-browser compatibility and prevents potential issues with live NodeList iteration. This is a best practice when iterating over DOM collections that might change during iteration.packages/cli/src/commands/results.ts (1)
241-275: No external imports ofBenchmarkOutputdetected; making it internal is safe.
Ran ripgrep across the repo and found zero matches for anyimport { BenchmarkOutput }orimport type { BenchmarkOutput }.benchmarks/with-ethyca/app/page.tsx (1)
1-7: Minimal Next.js page component looks correctSimple server component with static markup is fine for a benchmark entry point. No issues spotted.
benchmarks/with-ethyca/tsconfig.json (1)
1-11: TS config aligns with Next.js project conventionsExtends a shared Next config, includes Next’s generated types, and sets a common @/* alias. Looks good.
benchmarks/with-cookie-yes/package.json (2)
13-15: Dependency bumps look consistent and compatibleUpgrading next to 15.4.6 and react/react-dom to 19.1.1 is consistent with the rest of the PR. No concerns here.
19-24: Confirmedbenchmark-clibinary in @cookiebench/cli
Thepackages/cli/package.jsondeclares:"bin": { "benchmark-cli": "dist/index.mjs" }All
pnpm exec benchmark-cli …invocations will resolve correctly. No changes needed.benchmarks/with-osano/package.json (1)
13-15: Version bumps look goodDependencies updated consistently with the rest of the repo. No runtime changes here.
benchmarks/baseline/package.json (1)
14-16: Baseline dependency updates are consistentAligns Next and React versions with other benchmarks. Good to go.
benchmarks/with-ethyca/next-env.d.ts (1)
1-5: Standard Next.js ambient types file addedMatches Next’s guidance; nothing to change.
benchmarks/with-didomi/package.json (1)
14-16: Dependency versions aligned with the repo-wide updateNext 15.4.6 and React 19.1.1 versions match the rest of the suite. Looks good.
benchmarks/with-cookie-control/package.json (1)
19-24: CLI binary name check (same note as other packages)Scripts call
benchmark-cli. Please verify@cookiebench/cliexposes that binary so the script works across all benchmark packages.You can reuse the earlier verification script to check the bin mapping and search occurrences.
benchmarks/with-enzuzo/package.json (1)
13-24: LGTM on dependency bumpsVersion upgrades to Next 15.4.6, React 19.1.1, and TS 5.9.2 look consistent with the wider repo changes. No issues spotted.
benchmarks/with-c15t-nextjs/package.json (1)
14-26: Peer Dependencies Are CompatibleThe selected versions satisfy @c15t/nextjs@1.5.0’s peer ranges:
- Next 15.4.6 matches
^15.0.0- React 19.1.1 and React DOM 19.1.1 match
^19.0.0No further changes are required.
benchmarks/with-iubenda/package.json (1)
13-24: LGTM on version updatesBumps align with the rest of the benchmarks. No functional concerns.
benchmarks/with-usercentrics/package.json (1)
13-24: LGTMDependencies and types updated consistently; no functional code impact.
benchmarks/with-c15t-react/package.json (1)
13-26: Peer support confirmed for React 19 in @c15t/react; no peers declared for @c15t/translations
@c15t/react 1.5.0 declares
• react: ^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 || ^19.0.0-rc
• react-dom: ^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0 || ^19.0.0-rc
(includes React 19)@c15t/translations 1.5.0 has no peerDependencies — no install-time peer warnings expected
package.json (1)
15-24: Consistent TypeScript and Playwright versions detectedAll workspaces specify TypeScript 5.9.2 (leaf packages use
^5.9.2, root uses exact5.9.2), so only one TS version will be hoisted. Likewise,@playwright/testappears only in the root andpackages/cli, both at^1.54.2. No mismatched majors or risk of multiple runner versions.benchmarks/with-ethyca/config.json (2)
2-2: Dependency Verified: @cookiebench/benchmark-schema Is DeclaredI’ve confirmed that
@cookiebench/benchmark-schemaappears inbenchmarks/with-ethyca/package.jsonas a workspace dependency (workspace:*), so the$schemapath (./node_modules/@cookiebench/benchmark-schema/schema.json) will resolve correctly in editors.
41-42: Unable to locate benchmark schema – please verifyincludes.backendagainst the expected type
We couldn’t find@cookiebench/benchmark-schema/schema.jsonin your workspace, so the field’s expected type remains unclear. Ensure the schema package is installed (npm install @cookiebench/benchmark-schemainbenchmarks/with-ethyca) or point to the correct schema file, then confirm thatincludes.backendmatches its definition (e.g. boolean or object) rather than holding a license string.• File:
benchmarks/with-ethyca/config.json
Lines 41–42:"backend": "Apache-2.0 license", "components": ["python"]• If the schema expects a boolean or object, replace the license string with the appropriate value.
• If you intend to capture license metadata here, either update the schema to allow strings or rename this field to avoid confusion.benchmarks/with-ketch/package.json (2)
13-16: Deps bump LGTM; check Next/React peer compatibilityUpgrades to Next 15.4.6 and React 19.1.1 look consistent with the repo-wide changes.
20-24: Dev type bumps LGTM; align TS across workspaceThese dev type updates align with the TS bump. Confirm all packages resolve to the same TS major.
Use the script in the root package.json review to confirm TS version consistency across packages.
packages/cli/package.json (1)
29-34: Build toolchain bumps LGTM@rslib/core, @rsdoctor/rspack-plugin, @types/node, and typescript updates look aligned with the rest of the repo.
biome.jsonc (2)
12-12: Adoption of Biome 2’s assist.organizeImports looks goodEnabling organizeImports via assist is aligned with Biome 2.x config.
47-56: No action needed: “useFilenamingConvention” is correct in Biome v2.1.x
I verified via the official docs and v2.x changelog that the rule identifier remainsuseFilenamingConvention. No renaming touseFileNameConventionhas occurred.
| "@types/node": "^24.2.1", | ||
| "@types/react": "^19.1.9", | ||
| "@types/react-dom": "^19.1.7", | ||
| "typescript": "^5.9.2" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Types and TS upgrades are OK; ensure Node version alignment
Upgrading @types/node to ^24.2.1 and TypeScript to ^5.9.2 is fine. Please ensure the repo’s Node engine (CI and dev) aligns with the chosen type definitions to avoid type/env mismatches.
Run this to check engines and Node usage hints across the repo:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files declaring engines:"
rg -n '"engines"\\s*:'
echo
echo "Node versions mentioned in the repo:"
rg -n 'node\\s*[>=]*\\s*[0-9]'
echo
echo "nvm or volta configs (if any):"
fd -a '.nvmrc' '.node-version' 'volta.json' 2>/dev/null || trueLength of output: 103
Add a Node engine declaration to lock down your runtime
We didn’t find any engines entries in package.json nor any .nvmrc / .node-version / volta.json files, so CI/dev installs may pick up whatever Node is on the machine—potentially mismatching the APIs in @types/node@^24.2.1. Please add (or update) an engines field in your root package.json (and in benchmarks/baseline/package.json if it’s published or run standalone) to enforce your minimum Node version. For example:
{
"name": "your-project",
+ "engines": {
+ "node": ">=18.0.0"
+ },
"dependencies": { … }
}• Adjust the version range to match your support policy• If you use nvm or volta, add a matching .nvmrc / .node-version / volta.json so local and CI installs stay in sync
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In benchmarks/baseline/package.json (around lines 22-25) there is no "engines"
field; add an engines entry to declare the minimum Node version your package
requires (for example set node >=18.0.0 or the exact range your project
supports) so installs/CI use a compatible runtime, update the root package.json
with the same engines range if the repo is the source of truth, and add a
matching .nvmrc / .node-version / volta.json file if you use nvm/volta to keep
local and CI toolchains in sync.
| <Script id="fides-loader" strategy="beforeInteractive"> | ||
| {` | ||
| function loadFides () { | ||
| if (document.getElementById('fides-js')) return; | ||
|
|
||
| window.fides_overrides = { | ||
| fides_consent_non_applicable_flag_mode: "include", | ||
| fides_consent_flag_type: "boolean", | ||
| }; | ||
| addEventListener("FidesInitializing", function () { | ||
| window.Fides.gtm({ | ||
| non_applicable_flag_mode: "include", | ||
| flag_type: "boolean", | ||
| }); | ||
| }); | ||
| addEventListener("FidesInitialized", function () { | ||
| var id = window.Fides?.experience?.experience_config?.id; | ||
| if (id) document.body.classList.add(id); | ||
| }); | ||
|
|
||
| var fidesPrefix = "fides_"; | ||
| var searchParams = new URLSearchParams(location.search); | ||
| var fidesSearchParams = new URLSearchParams(); | ||
| for (var entry of searchParams.entries()) { | ||
| var key = entry[0], value = entry[1]; | ||
| if (key.startsWith(fidesPrefix)) { | ||
| fidesSearchParams.set( | ||
| key.replace(fidesPrefix, ""), | ||
| key === fidesPrefix + "cache_bust" ? Date.now().toString() : value | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| var src = "https://ethyca.fides-cdn.ethyca.com/fides.js?" | ||
| + fidesSearchParams.toString() | ||
| + "&property_id=FDS-KSB4MF"; // replace if needed | ||
|
|
||
| var s = document.createElement("script"); | ||
| s.id = "fides-js"; | ||
| s.async = false; | ||
| s.defer = false; | ||
| s.src = src; | ||
| document.head.appendChild(s); | ||
| } | ||
|
|
||
| loadFides(); | ||
| `} | ||
| </Script> | ||
| </head> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coding property_id; inject from env to keep config flexible
Replace the inline “FDS-KSB4MF” with a compile-time NEXT_PUBLIC env to prevent hard-coding IDs in source.
Apply this diff:
- var src = "https://ethyca.fides-cdn.ethyca.com/fides.js?"
- + fidesSearchParams.toString()
- + "&property_id=FDS-KSB4MF"; // replace if needed
+ var src = "https://ethyca.fides-cdn.ethyca.com/fides.js?"
+ + fidesSearchParams.toString()
+ + "&property_id=${process.env.NEXT_PUBLIC_FIDES_PROPERTY_ID ?? "FDS-KSB4MF"}";Follow-up:
- Document NEXT_PUBLIC_FIDES_PROPERTY_ID in README/benchmark notes.
- If you run with CSP, consider setting a Script nonce via Next config to permit this inline script.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Script id="fides-loader" strategy="beforeInteractive"> | |
| {` | |
| function loadFides () { | |
| if (document.getElementById('fides-js')) return; | |
| window.fides_overrides = { | |
| fides_consent_non_applicable_flag_mode: "include", | |
| fides_consent_flag_type: "boolean", | |
| }; | |
| addEventListener("FidesInitializing", function () { | |
| window.Fides.gtm({ | |
| non_applicable_flag_mode: "include", | |
| flag_type: "boolean", | |
| }); | |
| }); | |
| addEventListener("FidesInitialized", function () { | |
| var id = window.Fides?.experience?.experience_config?.id; | |
| if (id) document.body.classList.add(id); | |
| }); | |
| var fidesPrefix = "fides_"; | |
| var searchParams = new URLSearchParams(location.search); | |
| var fidesSearchParams = new URLSearchParams(); | |
| for (var entry of searchParams.entries()) { | |
| var key = entry[0], value = entry[1]; | |
| if (key.startsWith(fidesPrefix)) { | |
| fidesSearchParams.set( | |
| key.replace(fidesPrefix, ""), | |
| key === fidesPrefix + "cache_bust" ? Date.now().toString() : value | |
| ); | |
| } | |
| } | |
| var src = "https://ethyca.fides-cdn.ethyca.com/fides.js?" | |
| + fidesSearchParams.toString() | |
| + "&property_id=FDS-KSB4MF"; // replace if needed | |
| var s = document.createElement("script"); | |
| s.id = "fides-js"; | |
| s.async = false; | |
| s.defer = false; | |
| s.src = src; | |
| document.head.appendChild(s); | |
| } | |
| loadFides(); | |
| `} | |
| </Script> | |
| </head> | |
| <Script id="fides-loader" strategy="beforeInteractive"> | |
| {` | |
| function loadFides () { | |
| if (document.getElementById('fides-js')) return; | |
| window.fides_overrides = { | |
| fides_consent_non_applicable_flag_mode: "include", | |
| fides_consent_flag_type: "boolean", | |
| }; | |
| addEventListener("FidesInitializing", function () { | |
| window.Fides.gtm({ | |
| non_applicable_flag_mode: "include", | |
| flag_type: "boolean", | |
| }); | |
| }); | |
| addEventListener("FidesInitialized", function () { | |
| var id = window.Fides?.experience?.experience_config?.id; | |
| if (id) document.body.classList.add(id); | |
| }); | |
| var fidesPrefix = "fides_"; | |
| var searchParams = new URLSearchParams(location.search); | |
| var fidesSearchParams = new URLSearchParams(); | |
| for (var entry of searchParams.entries()) { | |
| var key = entry[0], value = entry[1]; | |
| if (key.startsWith(fidesPrefix)) { | |
| fidesSearchParams.set( | |
| key.replace(fidesPrefix, ""), | |
| key === fidesPrefix + "cache_bust" ? Date.now().toString() : value | |
| ); | |
| } | |
| } | |
| var src = "https://ethyca.fides-cdn.ethyca.com/fides.js?" | |
| + fidesSearchParams.toString() | |
| + "&property_id=${process.env.NEXT_PUBLIC_FIDES_PROPERTY_ID ?? \"FDS-KSB4MF\"}"; | |
| var s = document.createElement("script"); | |
| s.id = "fides-js"; | |
| s.async = false; | |
| s.defer = false; | |
| s.src = src; | |
| document.head.appendChild(s); | |
| } | |
| loadFides(); | |
| `} | |
| </Script> | |
| </head> |
🤖 Prompt for AI Agents
In benchmarks/with-ethyca/app/layout.tsx around lines 7 to 55, the script
hard-codes the Fides property_id ("FDS-KSB4MF"); replace that literal with a
compile-time env var (NEXT_PUBLIC_FIDES_PROPERTY_ID) so the src string is built
from process.env.NEXT_PUBLIC_FIDES_PROPERTY_ID (or an empty/default fallback)
instead of the hard-coded value, ensure the env var is read at build-time in
Next (export NEXT_PUBLIC_FIDES_PROPERTY_ID in your .env or CI), update
README/benchmark notes to document NEXT_PUBLIC_FIDES_PROPERTY_ID, and if your
app uses a strict CSP configure a script nonce via Next config so this inline
script is allowed.
| "bundleType": "iffe", | ||
| "frameworks": [], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Typo: "iffe" should be "iife"
The bundle type acronym is IIFE. This likely won’t validate against the schema.
Apply this diff:
- "bundleType": "iffe",
+ "bundleType": "iife",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "bundleType": "iffe", | |
| "frameworks": [], | |
| "bundleType": "iife", | |
| "frameworks": [], |
🤖 Prompt for AI Agents
In benchmarks/with-ethyca/config.json around lines 19 to 20, the "bundleType"
value is misspelled as "iffe"; change it to the correct acronym "iife" so the
JSON matches the expected schema and validates (update the string value from
"iffe" to "iife").
| "languages": ["javascript"], | ||
| "packageManager": "unknown", | ||
| "typescript": false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tech stack metadata likely inaccurate (TypeScript false, languages)
This benchmark uses .ts/.tsx. Consider reflecting that in the config for accurate reporting.
- "frameworks": [],
- "languages": ["javascript"],
+ "frameworks": ["nextjs", "react"],
+ "languages": ["typescript", "javascript"],
"packageManager": "unknown",
- "typescript": false
+ "typescript": trueIf the schema restricts allowed “frameworks”, adjust accordingly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "languages": ["javascript"], | |
| "packageManager": "unknown", | |
| "typescript": false | |
| "frameworks": ["nextjs", "react"], | |
| "languages": ["typescript", "javascript"], | |
| "packageManager": "unknown", | |
| "typescript": true |
🤖 Prompt for AI Agents
In benchmarks/with-ethyca/config.json around lines 21-23, the tech-metadata is
incorrect for this benchmark which uses .ts/.tsx files: update "languages" to
include "typescript" (e.g., ["typescript","javascript"] or just ["typescript"]),
set "typescript" to true, and correct "packageManager" to the actual package
manager used (npm/pnpm/yarn); if the schema restricts allowed framework/language
values, pick the closest allowed entries or adjust the schema accordingly so the
config accurately reflects the TypeScript codebase.
| "dev": "next dev --port 3006 --turbopack", | ||
| "build": "next build --turbopack", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Port conflict: dev server uses 3006 (already used by with-onetrust)
Running both benchmarks concurrently will fail. Please assign a unique port.
Apply:
- "dev": "next dev --port 3006 --turbopack",
+ "dev": "next dev --port 3007 --turbopack",Use this to list all dev ports and spot duplicates:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob 'benchmarks/**/package.json' '"next dev --port [0-9]+"' | \
sed -E 's#^.*/benchmarks/([^/]+)/package.json:.*"next dev --port ([0-9]+).*#\2 \1#' | \
sort -n | awk '{printf "%s\t%s\n",$1,$2}'Length of output: 220
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# List all benchmarks and their dev ports
rg -n --glob 'benchmarks/**/package.json' 'next dev --port [0-9]+' | \
sed -E 's#^.*/benchmarks/([^/]+)/package.json:.*next dev --port ([0-9]+).*#\2\t\1#' | \
sort -nLength of output: 1123
Update with-ethyca dev port to avoid conflict
The script shows multiple benchmarks using port 3006 (with-ethyca, with-onetrust, with-osano). Assign a free port (e.g. 3007) to with-ethyca:
• benchmarks/with-ethyca/package.json (line 5)
- "dev": "next dev --port 3006 --turbopack",
+ "dev": "next dev --port 3007 --turbopack",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "dev": "next dev --port 3006 --turbopack", | |
| "build": "next build --turbopack", | |
| "dev": "next dev --port 3007 --turbopack", | |
| "build": "next build --turbopack", |
🤖 Prompt for AI Agents
In benchmarks/with-ethyca/package.json around lines 5 to 6, the dev script is
set to use port 3006 which conflicts with other benchmarks; update the "dev"
script to use an unused port (e.g., 3007) so it reads next dev --port 3007
--turbopack (leave the build script unchanged). Ensure package.json remains
valid JSON and update any README or local run instructions if they reference the
old port.
| "next": "15.4.6", | ||
| "react": "^19.1.1", | ||
| "react-dom": "^19.1.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@cookiebench/benchmark-schema": "workspace:*", | ||
| "@cookiebench/cli": "workspace:*", | ||
| "@cookiebench/ts-config": "workspace:*", | ||
| "@types/node": "^22.15.30", | ||
| "@types/react": "^19.1.6", | ||
| "@types/react-dom": "^19.1.6", | ||
| "typescript": "^5.8.3" | ||
| "@types/node": "^24.2.1", | ||
| "@types/react": "^19.1.9", | ||
| "@types/react-dom": "^19.1.7", | ||
| "typescript": "^5.9.2" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align version range policy and verify cross-package consistency
Next is pinned while React/ReactDOM use carets. Consider standardizing version policy across all benchmarks (either pin or caret) to reduce drift and lockfile churn. Also verify all benchmark packages use a single Next/React/TS toolchain to avoid accidental mixed majors.
Run this script to inventory versions across all benchmark package.json files:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List Next/React/ReactDOM/TS versions per package for quick visual consistency check
fd -t f package.json benchmarks | sort | while read -r f; do
jq -r --arg file "$f" '
"\($file) :: " +
"next=" + (.dependencies.next // "—") + " ; " +
"react=" + (.dependencies.react // "—") + " ; " +
"react-dom=" + (.dependencies["react-dom"] // "—") + " ; " +
"typescript=" + (.devDependencies.typescript // "—")
' "$f"
done
echo
echo "Unique Next versions:"
rg -n '"next":' -g 'benchmarks/**/package.json' | sed -E 's/^.*"next": "([^"]+)".*$/\1/' | sort -u
echo
echo "Unique React versions:"
rg -n '"react":' -g 'benchmarks/**/package.json' | sed -E 's/^.*"react": "([^"]+)".*$/\1/' | sort -u
echo
echo "Unique ReactDOM versions:"
rg -n '"react-dom":' -g 'benchmarks/**/package.json' | sed -E 's/^.*"react-dom": "([^"]+)".*$/\1/' | sort -u
echo
echo "Unique TypeScript versions:"
rg -n '"typescript":' -g 'benchmarks/**/package.json' | sed -E 's/^.*"typescript": "([^"]+)".*$/\1/' | sort -uLength of output: 6916
Standardize version range policy for benchmarks
All benchmark package.json files currently use the same toolchain versions but mix pin vs. caret ranges:
- Next: 15.4.6 (pinned)
- React: ^19.1.1
- ReactDOM: ^19.1.1
- TypeScript: ^5.9.2
Location:
- benchmarks/*/package.json
To reduce drift and lockfile churn, pick one policy and apply it uniformly across all benchmarks:
- Switch Next to a caret range:
"next": "^15.4.6" - Or pin React/ReactDOM/TypeScript exactly:
"react": "19.1.1","typescript": "5.9.2", etc.
🤖 Prompt for AI Agents
In benchmarks/with-onetrust/package.json around lines 13 to 24, the dependency
versions mix pinned and caret ranges which causes lockfile churn; update the
file to use a consistent policy across all benchmarks — either change "next":
"15.4.6" to a caret range ("^15.4.6") to match React/ReactDOM, or pin React,
ReactDOM and TypeScript (remove carets) to match Next; apply the same choice to
all other benchmarks/*/package.json files so every benchmark uses the same
version-range policy.
| "@playwright/test": "^1.54.2", | ||
| "cli-table3": "^0.6.3", | ||
| "dotenv": "^16.5.0", | ||
| "package-manager-detector": "^1.3.0", | ||
| "picocolors": "^1.0.0" | ||
| "dotenv": "^17.2.1", | ||
| "picocolors": "^1.0.0", | ||
| "pretty-ms": "^9.2.0" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Playwright/dotenv/prettier-ms changes: verify runtime compatibility
- @playwright/test to ^1.54.2 is fine with Node >=18; ensure CI runner images reflect this.
- dotenv major bump (16 → 17) can change ESM/CommonJS import semantics. This package is ESM ("type": "module"). Verify imports are still correct.
- pretty-ms added: ensure tree-shaken usage and no unnecessary bloat in the CLI binary.
Run this to find dotenv usages and any stale package-manager-detector imports:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Searching for dotenv usage in CLI..."
rg -n --hidden --glob '!dist' '(from\s+[\"\']dotenv[\"\']|dotenv\/config|require\([\"\']dotenv[\"\']\))' packages/cli -A2 -B2 || true
echo
echo "Searching for removed package 'package-manager-detector' usages..."
rg -n --hidden --glob '!dist' 'package-manager-detector' packages/cli -A2 -B2 && { echo "Found references. Please remove or replace."; exit 1; } || echo "No references found."Length of output: 212
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "🔍 Searching for dotenv usage in packages/cli..."
rg -n --hidden --glob '!dist' \
-e "import.*['\"]dotenv(['\"].*|/config['\"])" \
-e "require\(['\"]dotenv['\"]\)" \
-e "dotenv/config" \
packages/cli -A2 -B2 || echo "✅ No dotenv imports or config usages found."
echo
echo "🔍 Searching for package-manager-detector references..."
rg -n --hidden --glob '!dist' 'package-manager-detector' packages/cli -A2 -B2 \
&& { echo "❌ Found references to package-manager-detector; please remove or replace."; exit 1; } \
|| echo "✅ No package-manager-detector references found."Length of output: 1017
🌐 Web query:
Does dotenv version 17.2.1 support importing the named export `config` (e.g., `import { config } from 'dotenv'`) in ESM?
💡 Result:
No. In dotenv 17.x, the recommended ESM usage is either the side‑effect import import 'dotenv/config' or importing the module namespace and calling dotenv.config(); the package does not provide a stable named export called config for ESM like import { config } from 'dotenv'.[1][5]
- The official README for current dotenv shows ESM usage as:
- Side-effect import: import 'dotenv/config'[1][5]
- It explicitly marks import * as dotenv from 'dotenv'; dotenv.config() in ESM as an example of what people try but warn about ordering issues; it does not document a named export.[5]
- Node’s ESM interop can sometimes synthesize “named exports” from CommonJS based on static analysis, but this is not guaranteed and can break; the Node docs recommend relying on default or namespace import when consuming CJS.[10] dotenv itself is CommonJS, so any import { config } from 'dotenv' would rely on this heuristic, not on a real ESM export from dotenv.[10]
Citations:
- 1: https://www.npmjs.com/package/dotenv
- 2: https://www.darraghoriordan.com/2021/06/26/publishing-package-using-esm-esmodules
- 3: https://pypi.org/project/python-dotenv/
- 4: https://www.npmjs.com/package/dotenv-esm
- 5: https://github.com/motdotla/dotenv
- 6: importing dotenv using ES6 imports motdotla/dotenv#114
- 7: Importing dotenv in ES6 motdotla/dotenv#89
- 8: https://www.npmjs.com/package/dotenv/v/12.0.2
- 9: https://nirmalsankalana.hashnode.dev/how-to-use-env-with-es6-modules
- 10: https://nodejs.org/api/esm.html
Fix dotenv ESM import in CLI and validate Node version
-
packages/cli/src/commands/results.ts (around line 8) currently does:
import { config } from "dotenv"; // … config();
Dotenv 17.x is still CommonJS and does not provide a real named
configexport in ESM. This will break under strict ESM interop. Replace with one of:- import { config } from "dotenv"; - config(); + import "dotenv/config";
or
- import { config } from "dotenv"; + import * as dotenv from "dotenv"; + dotenv.config();
-
@playwright/test upgraded to ^1.54.2 requires Node ≥18. Make sure your CI runners (e.g. GitHub Actions, Docker images) are on Node 18+.
-
pretty-ms is now imported in results.ts. Verify its default import remains correct and that it doesn’t pull in extra overhead when bundling or packaging the CLI.
| try { | ||
| runDrizzleCommand('studio'); | ||
| } catch (error) { | ||
| } catch { | ||
| // Studio command might be interrupted by Ctrl+C, which is normal | ||
| p.log.info('Studio closed.'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bare catch is OK, but this try/catch is effectively unreachable with current error handling
Because runDrizzleCommand catches all errors and calls process.exit(1), this try/catch won’t be reached on failures (including Ctrl+C). If the goal is to show “Studio closed.” on Ctrl+C, you’ll need runDrizzleCommand to propagate errors (or at least SIGINT) instead of exiting.
Two paths:
- Minimal: remove the try/catch here to avoid misleading impression it will ever run.
- Better: refactor
runDrizzleCommandto throw (or specifically swallow SIGINT) and let callers decide.
Apply this minimal diff to remove the unreachable try/catch:
- try {
- runDrizzleCommand('studio');
- } catch {
- // Studio command might be interrupted by Ctrl+C, which is normal
- p.log.info('Studio closed.');
- }
+ runDrizzleCommand('studio');
+ // If drizzle-kit exits normally, just return.If you prefer the “better” route, I can propose a small runDrizzleCommand options signature to propagate or ignore SIGINT instead of exiting. Want me to draft that?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| runDrizzleCommand('studio'); | |
| } catch (error) { | |
| } catch { | |
| // Studio command might be interrupted by Ctrl+C, which is normal | |
| p.log.info('Studio closed.'); | |
| } | |
| runDrizzleCommand('studio'); | |
| // If drizzle-kit exits normally, just return. |
🤖 Prompt for AI Agents
In packages/cli/src/commands/db.ts around lines 174 to 179, the try/catch around
runDrizzleCommand('studio') is effectively unreachable because runDrizzleCommand
catches all errors and calls process.exit(1); remove the surrounding try/catch
so the code simply calls runDrizzleCommand('studio') directly to avoid
misleading error handling, or alternatively refactor runDrizzleCommand to
propagate errors (or specifically rethrow/ignore SIGINT) so callers can handle
Ctrl+C—for the minimal change, delete the try { ... } catch { ... } block and
leave a direct call.
| const networkScore = calculateNetworkScore(metricsData); | ||
| const transparencyScore = calculateTransparencyScore( |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Signature change acknowledged; double-check size units for network scoring (likely unit mismatch).
The calculateNetworkScore(metricsData) change is consistent with removing resource-based inputs. However, the function computes:
- totalSizeKB = totalSize / 1024
- thirdPartySizeKB = thirdPartySize / 1024
In packages/cli/src/commands/results.ts, results are displayed as “KB” (e.g., ${r.avgSize.toFixed(2)}KB), which strongly suggests size.total and related fields are already in KB. If true, dividing by 1024 again underestimates sizes and inflates scores (e.g., 300 KB becomes ~0.29 “KB” in the thresholds).
Please confirm the unit of bundleMetrics.totalSize and networkMetrics.thirdPartySize. If they’re already KB, remove the extra division by 1024 and adjust variable names to avoid confusion.
To verify, search for the unit semantics of the collected sizes:
If sizes are confirmed to be in KB, consider this change in calculateNetworkScore (outside the selected range):
// calculateNetworkScore: treat inputs as KiB, not bytes
const totalSizeKiB = totalSize; // already KB
const sizeScore =
totalSizeKiB <= 50 ? 35 : totalSizeKiB <= 100 ? 25 : totalSizeKiB <= 200 ? 15 : totalSizeKiB <= 500 ? 10 : 5;
const thirdPartySizeKiB = thirdPartySize; // already KB
const thirdPartyNetworkScore =
thirdPartySizeKiB === 0 ? 25 : thirdPartySizeKiB <= 50 ? 15 : thirdPartySizeKiB <= 100 ? 10 : 5;Alternatively, standardize on bytes end-to-end (preferred long-term), and multiply in the caller if needed so calculateNetworkScore always receives bytes.
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Look for how sizes are displayed or described in code/comments
rg -n -A2 -B2 "avgSize.*KB|size\.total|thirdParty.*size|formatBytes\\(totalSize\\)" -S
# Search for any comments or code that say 'bytes' vs 'KB'
rg -n -A2 -B2 "bytes|KB|KiB|kilobyte|byte" -SLength of output: 15857
Verify Units of Bundle and Network Sizes
It appears that:
bundleMetrics.jsSizeandbundleMetrics.cssSizecome straight from the bundler plugin and are in bytes.- In
network-monitor.ts, each request’ssizeis explicitly converted to KB (size: size / 1024).
Thus within calculateNetworkScore you’re mixing bytes (bundle sizes) and KB (network sizes) under the same umbrella. Then by dividing both totalSize and thirdPartySize by 1024, you:
- turn bundle sizes into KB (correct),
- turn already-KB network sizes into MB (incorrect),
leading to under‐reporting network weight and inflated network scores.
Suggested fixes:
- Normalize all inputs to bytes or to KB before scoring.
- If you prefer KB everywhere, remove
/ 1024in the third‐party block and rename those variables tototalSizeKBandthirdPartySizeKBaccordingly. - If you prefer bytes end-to-end, change network‐monitor to keep
sizein bytes (drop/ 1024), so the/ 1024in scoring applies uniformly.
Points of attention:
- packages/cli/src/utils/scoring.ts (lines 428–431, 450–453)
- packages/cli/src/commands/benchmark/network-monitor.ts (lines 31–32)
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores